feat(remote): serve a model on a tailnet GPU machine - #333

The red E2E tests (GPU) here is shared-runner state, not your change.

That job landed on mi300x-0, whose shared pre-warm tree had torch-2.11.0+rocm7.14.0 sitting inside the ROCm 7.13.0 runtime — a cross-wiring from an earlier run. Multi-arch wheels are published stripped of device code by design, so every vLLM start on that tree dies the same way:

RuntimeError: Engine core initialization failed. See root cause above.

which is exactly the three unexpected failures in this run (93 scenarios (85 passed, 8 failed), 5 of them the known xfails). Full diagnosis in #314.

The runner is repaired — torch is back to 2.11.0+rocm7.13.0 and a real kernel verified on it — and I have re-run the job, so nothing needed from you. E2E tests (Strix Halo, Ubuntu) is a separate failure that I have not looked at.

Addressed all 4 blocking findings and 4 of 5 non-blocking findings from the automated review; skipping one non-blocking item as a follow-up.

Blocking

• run.sh:84 — removed the || fallback to an all-interfaces bind. A failed loopback bind is now a hard error, so the password account never reaches the LAN.
• remote-09 (remote_steps.rs / remote.feature) — added an INCLUDE_ROCM_CLI build arg so the fixture image can be built without the rocm binary, and gave remote-09 a Given step that starts that variant. The scenario now reaches locate_cli's refusal for real, and then_doctor_points_at_serve's assertion is unconditional rather than permanently skipped.
• run-e2e.sh:195 — added a tailscale serve status --json check right after attach, so a re-publish that doesn't actually happen fails the harness instead of only checking printed strings.
• mod.rs stdin test — split serve() into serve_with_transport() so the test drives the real orchestration through a ScriptedTransport, instead of calling exec_with_stdin directly and asserting on its own input. Reverting the real call site's Some(&api_key) back to None now fails this test (checked by reverting it locally and confirming the failure, then restoring it).

Non-blocking

• provision.rs:130 — ROCM_CLI_SIGNING_PUBLIC_KEY_PATH/_PEM are now forwarded to the remote's install.sh, shell-quoted, so a private-mirror signing key actually reaches the remote instead of falling back to the pinned production key.
• provision.rs:155 — the archive path is now shell_quoted like every other interpolation in the module.
• transport.rs / tailnet.rs — dropped both stale dead_code attributes; this PR is the change their own comments said to remove them in.
• run-e2e.sh:141 — the sshd readiness loop now hard-fails with a message instead of falling through silently after 15s.
• ServerAliveInterval/ServerAliveCountMax (transport.rs:238) — left out of this pass; tracked as a follow-up rather than folded in here.

Verification

• cargo fmt --check, cargo clippy --workspace --all-targets -- -D warnings, and cargo test --workspace --all-targets all pass clean.
• Ran the e2e-cucumber remote-09 scenario against a real container built with no rocm binary — passes, 4/4 steps.
• Ran tests/remote-ssh/run-e2e.sh end to end against a real container — all checks pass, including the new post-attach publish check.
• Not verified: a real tailnet or a real GPU. Both harnesses remain the same container-based stand-ins used elsewhere in this PR.

Also replied to and resolved the two CodeQL threads: alerts #778/#779 already report state: fixed on this head.

🔴 Automated review · pr-review-watcher · 486054f

This automation posts comments only. It never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

Adds rocm remote — provision, serve, publish and tear down a model on a tailnet GPU machine over ssh — with unit, cucumber and container-backed e2e coverage. Outcome: Needs work — both prior blockers are genuinely fixed, but three new issues surfaced, two of them repeats of the same two defect classes one layer away from where they were fixed. Verified: ran cargo test -p rocm --bin rocm remote:: (130 passed, 0 failed) and read install.sh and provision.rs side by side — the signing-key precedence now genuinely matches (_PATH > _PEM > pinned in both, pinned by a_path_wins_over_a_pem_because_that_is_what_install_sh_does, which fails if reverted), the forwarded fragment really does blank the remote's _PATH, the head commit's doc walk-back is accurate to the code, transport.rs really does check ssh's 255 before the writer-thread result (an_unreachable_host_says_so_even_when_a_payload_was_being_written fails if reverted, and its 1 MiB payload makes the EPIPE deterministic rather than racy); leak scan across the diff is clean, all 19 commits are signed and carry a matching DCO sign-off, no prompt-injection content anywhere in the checkout. Blocking: 3 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

1. apps/rocm/src/main.rs:5972 — the --require-api-key guard is passed a hardcoded false, disabling it at this call site.

ensure_public_service_has_endpoint_key(host, endpoint_key_file.is_some(), false)?;

ensure_public_service_has_endpoint_key (main.rs:5701) has two branches: a public bind without a key, and requires_api_key && !key_present. The second is the one this PR adds for exactly the threat it introduces — a loopback bind is no longer "only this machine" once the remote republishes the port onto the tailnet. But record.requires_api_key is computed 40 lines earlier (main.rs:5931) and is in scope, and this call passes the literal false instead. The other three call sites (restart_internal_managed_service at main.rs:15805, and both sites in apps/rocmd/src/lib.rs) pass the real value; this one is the outlier.

It is reachable, not merely theoretical, because the two values are computed by different tests: record.requires_api_key comes from file existence, while endpoint_key_file is filtered by validity (endpoint_api_key_from_file) — a distinction the comment immediately above spells out as the reason an "empty or malformed key file would otherwise satisfy the guard". So an existing-but-invalid key file yields requires_api_key = true, key_present = false: precisely the case the new branch exists to catch, and the hardcoded false lets it spawn an unauthenticated listener for a service the user explicitly asked to require a key.

This also repeats prior finding 1's shape: the comment on the line above ("enforce the invariant here too rather than relying on every future caller having done so") claims the invariant is enforced, and only half of it is.

Fix: ensure_public_service_has_endpoint_key(host, endpoint_key_file.is_some(), record.requires_api_key)?; and add a test that a present-but-empty key file on a --require-api-key loopback service refuses to spawn.

2. apps/rocm/src/remote/mod.rs:685 — a definite remote failure is collapsed into "the machine could not be asked".

publish: publish::publish_state(transport, record.tailnet_port, record.remote_port).ok(),

publish_state (publish.rs:132-148) returns Err in two materially different cases: the transport failed, or the remote was reached and tailscale serve status --json exited non-zero, in which case the error carries the exit code and the remote's own stderr (e.g. tailscale: command not found). .ok() discards both into None, which render_status (mod.rs:770) prints as "unknown — the machine could not be asked" — telling the user the machine was never asked when in fact it answered with a concrete, actionable reason.

This is the same defect class as prior blocking finding 2, one layer up and still present. The inconsistency is visible within the same function: forty lines earlier the very same code path carefully separates ServerHealth::Error (reached, failed) from ServerHealth::Unreachable (never reached), and the module docs at mod.rs:17-22 make that separation the stated design. Nothing in the test suite covers it — no test scripts a successful services list followed by a failing serve status.

Fix: carry the error rather than dropping it — make the field Result<PublishState, String> (or add a PublishState::Unreachable(String)), have render_status print the captured stderr, and add a ScriptedTransport test pinning that a reached-but-failed serve status is reported differently from an unreachable host.

3. tests/e2e-cucumber/tests/e2e/remote_steps.rs:297-305 — a Then step that asserts nothing when its guard does not match.

async fn then_doctor_points_at_serve(world: &mut E2eWorld) {
    let said = said(world);
    if said.contains("only reads") {
        assert!(said.contains("rocm remote serve"), "{said}");
    }
}

If the output does not contain "only reads", the step passes having verified nothing. Its partner step at remote_steps.rs:291-294 has the matching escape hatch (said.contains("only reads") || said.contains("could not reach")), and the preceding assert_ne!(cli_rc, Some(0)) is satisfied by any failure. Together they mean scenario remote-09 can go fully green while asserting only "the command exited non-zero somehow" — CI stays green as the coverage silently disappears.

This is the standing test-vacuity failure mode, and it is in the remediation itself: commit 6f3a81e (test(remote): give remote-09 a reachable container to check) made the container reachable precisely so the real branch is taken, but left the tolerance for unreachability in place. With reachability now guaranteed by given_reachable_machine_without_cli, the fallbacks are dead permissiveness.

Fix: drop both escape hatches — assert said.contains("rocm remote serve") unconditionally, and drop || said.contains("could not reach") — so an unexpected path fails loudly instead of passing quietly.

Non-blocking

• apps/rocm/src/remote/provision.rs:173 — remove_dir_all(&staging) is only reached on success; every ?/bail! above it leaks the staging dir, so repeated failed --install-rocm runs accumulate under $TMPDIR (0700 + nonce-named, so not a security issue — but the asymmetry reads as an oversight, not the deliberate keep-it-for-debugging choice it might be).
• apps/rocm/src/remote/provision.rs:96-113 — run_remote_installer, the path tried first, never forwards the signing override, so the "the key we send wins" guarantee only engages on the fallback; behaviour is fail-safe but nothing says so, and a reader of install_cli would reasonably assume it applies throughout. One comment line fixes it.
• apps/rocm/src/remote/provision.rs:160-166 — only the pure fragment builder is tested; nothing pins that {signing_env} is actually prefixed onto the remote command, so dropping it in a refactor would pass the whole suite.
• apps/rocm/src/remote/transport.rs:339-366 — the stdin/stdout deadlock fix is correct by inspection (writer thread + immediate wait_with_output), but no test exercises a reachable host that both consumes a large stdin and floods stdout; every current test passes if the fix is reverted.
• tests/e2e-cucumber/tests/e2e/remote_steps.rs:362-368 — free_port() binds, reads the port, drops the listener, then hands it to docker run; under the 64-way max_concurrent_scenarios this lane uses, that TOCTOU is a plausible intermittent-flake source. Note the CI state here is 19 success / 1 failure / 1 skipped: I cannot see which lane is red and am not claiming this is the cause — that would be an inference I have no way to confirm from the checkout.

Addressed the second review round.

Blocking finding 1 — publish.rs AllowFunnel blind spot. Fixed. AllowFunnel (keyed host:port) is now parsed into RawServeConfig; when set for the target port, classify() returns a new PublishState::FunnelAllowed, and both publish() and withdraw() bail loudly naming tailscale funnel --tcp=<port> off. SERVE_CONFIG_KEYS now has a comment distinguishing parsed keys from document-shape-only keys. Corrected the module doc's "visible to the whole tailnet, scoped only by its ACLs" claim, which Funnel already contradicted.

Blocking finding 2 — bootstrap.rs install-before-tailscale-check. Fixed. The tailscale_present check is now hoisted above the rocm_present branch, so a machine without Tailscale is refused before any install runs, regardless of what else is missing. Extended a_machine_without_tailscale_is_refused_before_a_model_is_started with an install_rocm = true, neither-present fixture; confirmed it fails without the hoist.

Non-blocking items — none skipped, all five addressed:

• provision.rs:164 — now reads _PATH locally and forwards its content as _PEM; an explicit _PEM still wins, matching install.sh's own resolution order. Split into a pure, dependency-injected helper with unit tests for all four cases (neither set, explicit-PEM-wins, PATH-read-and-forwarded, PATH-read-failure-reported).
• publish.rs:34,144 — the Services key had the same blind spot as AllowFunnel; fixed alongside finding 1.
• transport.rs:291 — fixed. The stdin write now happens on its own thread instead of being sequenced before wait_with_output(), removing the deadlock risk for a payload larger than the OS pipe buffer paired with remote output.
• provision.rs:234 — the staging dir is now salted with a nanosecond nonce and created with create_dir (not _all), so a pre-staged/symlinked path can't be silently adopted; restricted to 0700.
• transport.rs:269 — scp_argv now rejects a local_path/remote_path starting with -, mirroring validate_destination's existing guard for the ssh destination.

Verification (local, on top of the branch's current merge with main):

• cargo fmt --all -- --check — clean
• cargo clippy --workspace --all-targets -- -D warnings — zero warnings
• cargo test --workspace --all-targets — every test result: line workspace-wide reports 0 failed

Commits: 0e7adb38, e3613fe2, 2cc06956, de914c0c.

One open item, not part of this review round's findings: origin/main advanced again after these commits were prepared (one new commit, a9937493), and merging it into this branch is currently blocked by pre-existing staged, uncommitted changes in this worktree unrelated to this review (including staged deletions of two source files). That's being sorted out separately and isn't a gap in this review response.

Addressed the review on de914c0c, plus a rebase onto main. Force-pushed, so the review's line references point at commits that no longer exist — summary of what moved:

Both blocking findings fixed, each reproduced first.

• Signing-key precedence. _PATH now resolves before _PEM, matching resolve_public_keys, and an empty value counts as unset on both sides. The test that encoded the old order is replaced rather than kept. Verified by reverting each half separately: the old order yields pem-content where install.sh would use path-content, and without the empty-value filter an empty _PEM forwards PEM='' and silently discards the operator's real key.
• Transport ordering. The outcome is built and the 255 check runs before the writer result, which is now demoted to a symptom only when the command also failed. The regression test needed a payload larger than a pipe buffer — a short one lands in the buffer and returns success with no reader, so the bug is timing-dependent at that size and the test passed against the broken code. At 1 MiB it fails 3/3 before and passes 3/3 after.

All five non-blocking items fixed, including the scp guard rationale (the remote argument is prefixed with the destination, so that half is a shape check rather than a safety one) and the Funnel status line.

One finding I did not take. The CI lane suggestion assumed the missing toolchain explained the red check. It did not — remote control channel (containerised) was already passing; the failure was clippy, from a semantic conflict with main over resolve's signature. Fixed by rebasing and updating the three call sites. The toolchain step is still added, as a consistency fix.

Beyond the review, worth flagging:

• Forwarding a signing key now also blanks the remote's own ROCM_CLI_SIGNING_PUBLIC_KEY_PATH. Without it a key could be forwarded correctly and still lose, since resolve_public_keys reads _PATH first and /etc/environment reaches non-interactive sshd sessions.
• The container fake accepted funnel --tcp=8000, but Funnel serves only 443/8443/10000 — it was encoding a state tailscaled cannot emit. Fake and fixtures now use real Funnel ports. That surfaced the open question now in the PR description: the guard is unreachable at the default tailnet port.

Local: full workspace tests, clippy -D warnings, fmt, prek, and both container lanes (21 and 25 checks) all pass on the rebased tree.

CI status: 20 of 21 checks green, including clippy (the one that was red before the rebase), remote control channel (containerised), build-and-test, windows-build-and-test and Commit signatures + sign-off.

The one red check, E2E tests, is inherited from main and not from this branch:

• It fails on exactly two scenarios, dash-gen-tps-held-after-scrape-failure and dash-gen-tps-expiry-boundary.
• The same two fail on main's own HEAD (a6fa76db) with an identical reconciliation line. The commit before it (a9937493) was green, so the regression arrived with feat(therock): support ROCm 10 "next" install layout, additive (EAI-8431) #329.
• Re-ran the job to rule out a flake — same two scenarios, deterministically.
• This branch touches no dash code. git diff --name-only origin/main...HEAD returns nothing matching dash. The only file this branch shares with that area is expectation.rs, and the three call sites changed here are inside #[cfg(test)], so they cannot affect scenario resolution at runtime.

Flagging rather than fixing: a dash regression is unrelated to this PR and belongs in its own change. Happy to pick it up separately if that is useful.

History rewritten: 19 commits squashed to 3, and rebased onto current main. The tree is byte-identical to the pre-squash one (same tree hash) — only the history changed. Line anchors in the older review comments no longer resolve; the reasoning is preserved in this thread.

The three commits are feature / tests / docs. Further review rounds will amend these rather than stack more fix commits on top.

The three blocking findings — all verified, and all older than the last round. git log -L puts each on a 2026-09-01 commit, i.e. the PR's original work, not the remediation. They were in the tree at the previously reviewed head and were not reported then, so this is the review surfacing a deeper slice rather than a regression.

1. The managed-spawn guard was passed a literal false. Fixed to pass record.requires_api_key. The test is the part that mattered: a unit test on the guard function cannot catch this — the function was always correct, the wiring was not. The new test plants an empty key file, which is the only state where the guard's two inputs disagree (existence says a key is required, validity says none is present), and drives spawn_managed_engine_child on a loopback host so the public-bind branch cannot be what refuses it. Confirmed it fails with the literal restored.
2. publish_state(..).ok() collapsed reached-but-failed into never-reached. The same function makes exactly that distinction 28 lines earlier for services list, with a comment saying why. Added PublishObservation mirroring the ServerHealth split; publish_state keeps its signature so the four callers in publish.rs are untouched. status now prints the remote's own words. Confirmed the test fails when the collapse is restored.
3. Two dead escape hatches in remote-09. Both steps belong to that scenario alone, whose Given starts a real container, so the unreachable branch was unreachable. Dropped; the scenario still passes, which is what shows the hatches were carrying nothing.

All five non-blocking items are also fixed, including a Drop guard so the staging directory is removed on the error paths too, and a real test for the stdin/stdout deadlock fix. That last one is worth a note: my first attempt ran the child's output and input concurrently, and it passed with the fix reverted — i.e. it tested nothing. Sequencing the flood before the read reproduces the hang, and it now times out at 10s reverted and passes in 0.03s fixed.

Beyond the named findings, I swept the diff for each defect class rather than just the sites reported. That turned up one more instance of the same collapse: the session-listing path built a SessionObservation with ServerHealth::Error after SshTransport::new failed — but Error means "answered unreadably" and nothing had been sent, so it is Unreachable, and the constructor's reason was being discarded. Both fixed. The comment sweep and the literal-argument sweep came back clean.

Verified locally on the rebased tree: full workspace tests, clippy -D warnings, fmt, prek, both container lanes (21 and 25 checks), and all 14 remote cucumber scenarios with E2E_INCLUDE_DOCKER=1.

Pushed b014f528 → 15e42f0f. Both inline findings are fixed and answered in their threads; this covers the rest of what changed, since it is more than those two.

Found while verifying the two reported bugs

• provision.rs — the push-provisioning path never worked. ROCM_CLI_ARCHIVE was built by quoting {remote_dir}/{asset} as one value, and remote_dir is $HOME/.rocm/provision, deliberately left unexpanded for the remote shell. Single-quoting suppressed that expansion, so the remote looked for a file whose name literally began $HOME and failed at [ -f "${LOCAL_ARCHIVE}" ]. Only the asset name is quoted now. The old test asserted command.contains("ROCM_CLI_ARCHIVE="), which passed against the bug; the new one asks a real shell what the assignment evaluates to.
• main.rs / rocmd — requires_api_key was derived from key-file presence, but resolve_endpoint_auth mints a key for every non-loopback bind whether or not auth was demanded. So a plain --host 0.0.0.0 --allow-public-bind recorded true, and since the guard tests that field before the bind address, those services were refused with a message naming a flag they never passed and a relaunch command that drops --allow-public-bind — coming back on loopback instead. The real flag is threaded through the spawn chain now, and the daemon's copy lost the same || key-file-present clause.
• doctor.rs — suggested fix commands were interpolated into ssh <target> -- <command> unquoted. Most entries in the fix catalog contain a pipe, && or a redirect, so the command split across two machines: ssh box -- echo /usr/lib/wsl/lib | sudo tee /etc/ld.so.conf.d/wsl.conf runs the echo on the remote and the privileged tee locally. Rewriting is now done per Fix.commands entry rather than per rendered line, so multi-line entries stay whole, comment-only entries are not presented as commands, and the one stateful entry is not rewritten at all.
• tailnet.rs — check: rocm remote doctor {peer.host} rendered a self-reported HostName as copy-pasteable text. Any device on the tailnet chooses its own, and nothing quoted it. Now quoted, with a test that asks a real shell what the rendered line passes along.
• mod.rs — resolve_peer returned the first of several exact matches while its own doc comment said ambiguity is an error; the loose path already refused. HostName is not unique on a tailnet. It now refuses and names the candidates by DNS name.
• mod.rs — observe reported a machine that answered but whose command failed as Unreachable, rendering as "the machine could not be asked". It asks about publishing now, which is a fact it can still read.
• expectation.rs, session.rs — three doc comments had rebound onto newly inserted items, leaving the items they described undocumented and Included carrying resolve's algorithm docs plus parameter names that are now fields.
• run.sh — the credential-non-leak check read last-argv with || true, so an unreadable file yielded an empty string that trivially contains no key and the check passed having verified nothing. The e2e sibling did it correctly.
• remote_steps.rs — RemoteMachine::exec never checked exit status, and its caller asserts a port is absent from the serve config, which passes trivially on empty output.

Verification

cargo test --workspace --all-targets, cargo clippy --workspace --all-targets -- -D warnings, cargo fmt --check and prek all pass. Every new test was checked by reverting its fix and confirming it fails — one had to be re-shaped after that check showed it passing against reverted code, and one needed a timeout because a fail-open guard does not return an error, it falls through and hangs.

Still unverified, unchanged from the description: no real tailnet has carried traffic, no real GPU has loaded a model, and nothing here has spoken to a live tailscale daemon.

Not addressed, and worth a decision

• The remote control channel (containerised) lane is not in main's required checks and no job depends on it, so its checks cannot fail the build. Adding it needs the heavy gate moved from the job to its steps first — it is the only job-level one in the file, and a required check that never reports stalls the merge queue.
• serve --require-api-key and services list --json are user-observable and have no Gherkin scenario. AGENTS.md accepts a written justification instead, but not silence.

Pushed 15e42f0f → 357130b5. Both blocking findings are fixed.

1 — a port held by an HTTPS/HTTP handler read as free. Correct, and the test was pinning it. RawTcpHandler now parses HTTPS, HTTP and TerminateTLS alongside TCPForward, and classify treats any handler at the port that is not our matching forward as Foreign, naming what holds it so the refusal tells the user what they would have overwritten. A handler kind this CLI does not recognise refuses too — a newer Tailscale adding one must not read as an empty port. classify already resolves the handler across TCP, Foreground and Services before that point, so all three nestings are covered by the one change.

The test is rewritten to assert Foreign across all four handler shapes, plus the converse: a handler on a different port is still Absent, so the guard has not become "refuse whenever the document mentions a port". A second test drives publish end to end against a held port and asserts no serve --bg was issued.

The SERVE_CONFIG_KEYS doc comment is corrected, for the reason you gave — the HTTPS/HTTP fields are read not because this design ever terminates TLS, but because someone else's handler still occupies the port.

2 — the call-site test only defended one direction. Also correct. Added a case seeding two records, one requiring a key and one not, so the existing.service_id == record.service_id half of the predicate is load-bearing rather than decorative.

Worth recording how that went, because the first attempt failed in the way your finding predicts. I asserted on the service manifest appearing, as proof the guard had admitted the call — and both mutations you named survived it, because the seeding helper had already written that manifest, so the poll proved nothing. The working version decides on what the call returns, identifying the guard's refusal by its message rather than by the bare fact of an error. Confirmed by applying both mutations: each fails the test now, and it passes unmutated.

One more in the same area, not reported, found while acting on the first finding. fix-10-container stores a single docker run as seven Fix.commands entries, each ending in a trailing \. Rewriting per entry — correct for fix-1-arch, which is one entry with an embedded newline — turned it into seven independent ssh calls, each carrying a dangling backslash inside its quoting. Continued entries are now merged before anything rewrites them, so both storage shapes become one whole command first.

Also corrected a comment in provision.rs asserting that install.sh only requires a signature on the release channel. resolve_public_keys falls back to the pinned release key with no channel argument, so public_keys is non-empty on every channel and the || [ -n "${public_keys}" ] arm forces verification regardless of the channel-gated require_sig above it.

Non-blocking items: none addressed in this push, and taking them in turn rather than as a block — the explained_by_the_command demotion and the is_stateful_sequence coverage gaps are both real and both worth closing; remote_serve_command's missing -- is real and does fail closed as you say; the job-level heavy gate is real and is a prerequisite for making that lane a required check, which is a separate decision; and the traffic-level --require-api-key gap is inherent to the stand-in remote, so it belongs in the PR text as a named gap rather than behind a scenario that would only assert on CLI output.

Verification: cargo test --workspace --all-targets, cargo clippy --workspace --all-targets -- -D warnings, cargo fmt --check and prek all pass.

🔴 Automated review · pr-review-watcher · 357130b

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Scope note: this review was made against commit 357130b5, which was the head when it started. The branch has since moved on, so this is an observation about that commit and not a verdict on the current head. No change request is being filed on the strength of it; the newer commits will be looked at on their own terms.

Summary

Adds rocm remote — tailnet GPU discovery over SSH, health checks, provisioning, remote serving, and publishing the model's loopback port onto the tailnet — plus a container test lane and docs. Needs work: one test does not test what it names, everything else in a 31-file review came back clean. Verified: on a scratch copy outside the checkout I replaced RemoteSessionRecord::write's staging-file+rename with a plain fs::write to the destination and ran cargo test -p rocm --bin rocm remote::session — 9 passed, 0 failed, including a_record_is_replaced_atomically_so_a_reader_never_sees_half_of_one, so that test survives full removal of the atomicity mechanism it is named for. I also confirmed against the code that the API key is delivered on SSH stdin and never appears in the remote command string (so it is absent from ps and shell history), that production SSH never weakens host-key checking (no StrictHostKeyChecking=no/UserKnownHostsFile=/dev/null outside test fixtures), that checksum and signature verification in install.sh runs on both halves of the split download/install path before anything is executed, that the published forward is tailnet serve and never funnel with the model bound to 127.0.0.1 behind a required API key, and that every command, flag and security claim in the README and docs/testing.md matches the clap definitions and the harness. Both previously-filed counts are fixed at this head and the fixes are test-enforced: the port-classification lead now parses HTTPS/HTTP/TerminateTLS in all three nestings and classifies a non-matching handler as foreign, and the API-key call-site lead now has a two-record test that goes red both when the call site is hardcoded to true and when the per-service id comparison is dropped (both mutations run and confirmed red). Check runs at this head: 13 success, 11 pending, 1 neutral, 1 skipped — no failures, but most lanes had not reported when this review was made, so nothing here rests on CI. The full suite, the e2e suite and --all-targets were not run here. I confirmed every file in git diff prw-base...HEAD --name-only landed in a reviewer's scope — all 31, with the provisioning cluster re-dispatched after its first pass stalled, and nothing assumed covered by a sibling. The checkout was left unmodified (git status clean, HEAD still 357130b); all mutation work was done on a copy elsewhere. Blocking: 1 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

apps/rocm/src/remote/session.rs:369-395 — a_record_is_replaced_atomically_so_a_reader_never_sees_half_of_one cannot fail for the defect it names.

The test's comment states the guarded failure is a torn record that load_all skips, "which hides a session whose model is running and whose endpoint is published — the one thing that must never happen quietly." But the test performs two sequential writes and then asserts only that (a) exactly one record exists with the second write's model, and (b) no *tmp* file is left in the directory. Neither assertion involves concurrency, a partial write, or a reader observing an intermediate state.

Replace the whole mechanism at session.rs:125-132 — the path.with_extension(format!("{}.{}.tmp", ...)) staging file, the fs::write(&staging, ...), and the fs::rename(&staging, &path) — with a single fs::write(&path, &bytes), and the test still passes: the second sequential write still leaves the correct final content, and no staging file is ever created, so the leftover check trivially holds. I ran exactly this mutation on a scratch copy; all 9 remote::session tests passed. CI therefore certifies an atomicity guarantee no test exercises, and a future refactor that drops the rename would go unnoticed.

Fix: make the test observe the intermediate state. Either spawn two writer threads racing on the same session_id alongside a reader thread that asserts every observation is well-formed JSON matching one of the two writes (never a truncated blob), or — cheaper and deterministic — assert the staging path itself: write a record, and in the same test assert that a plain fs::write to a same-named destination while a partial file exists is never what load_all observes, by pre-creating a truncated <id>.json and proving a subsequent write() replaces it wholesale rather than appending into it. At minimum, add an assertion that a staging file with the {pid}.{millis}.tmp shape is created during the write (via a test seam), so deleting the rename step turns the suite red. Until then the test should not carry atomically in its name.

Non-blocking

• apps/rocm/src/remote/mod.rs:224 — the comment says "the assistant path allowlists this command as read-only" without naming which binary; the allowlist is in apps/rocmd/src/lib.rs:2527, and this ambiguity sent a reviewer here to the wrong file and the wrong conclusion. Name the binary.
• apps/rocmd/src/lib.rs:2527 vs apps/rocm/src/main.rs:12045 — the daemon now admits remote targets|doctor|status as read-only, but the CLI's own chat classifier has no remote arm and rejects it as unsupported; the adjacent setup arm's comment claims the two allowlists are mirrored across binaries, so this PR introduces a divergence from a stated invariant.
• apps/rocm/src/main.rs:444 — --api-key's help still says "Ignored for loopback binds, which stay credential-free", which stops being true once the new --require-api-key is passed (resolve_endpoint_auth at 5618 short-circuits only when !required).
• apps/rocm/src/remote/session.rs:125 — the staging suffix {pid}.{unix_millis}.tmp can collide between two threads in one process writing the same session id within a millisecond; add a counter or thread id.
• tests/e2e-cucumber/src/bin/fake-tailscale.rs and tests/remote-ssh/fake-tailscale.sh — the two stand-ins deliberately cover disjoint subcommands (local status vs remote serve/funnel), which reads as drift without a cross-reference comment in each; they also name the same concept differently (FAKE_TAILSCALE_STATUS vs FAKE_LOCAL_TAILSCALE_STATUS).

🔴 Automated review · pr-review-watcher · 10aaa17

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Scope note: this review judges commit 10aaa172 only. A newer commit (9c8edd95) was pushed while it was running, so this is not a verdict on the current head, and no review gate has been filed against it. The finding below was re-confirmed by execution at 10aaa172; it should be re-checked against the newer commit.

Summary

Adds rocm remote — serving a model on a remote GPU machine over SSH and publishing it to a tailnet — plus a containerised SSH harness, cucumber scenarios and docs. Needs work: one carried-over test-quality blocker is now confirmed by execution, everything else is a coverage or consistency gap. Verified: seven branch-level mutations run in a scratch copy against the two narrow unit modules they touch (remote::session::, remote::publish::, remote::doctor::, rocmd --lib) — six were caught, one survived (the atomic-write test below); the full suite, e2e and clippy were not run here. Also confirmed at this head, on the live code: the endpoint API key is minted, stored 0600 and piped on stdin, never appearing in the remote command string, argv or any error/log text (mod.rs:567-582, transport.rs stdin test); StrictHostKeyChecking/UserKnownHostsFile weakening exists only under tests/, never in apps/ (transport.rs:256-270 sets only BatchMode, ConnectTimeout, -p); checksum verification is unconditional and signature verification is forced on the release channel, both strictly before extraction or download-only exit (install.sh:412-460); the forward is hard-bound to 127.0.0.1 with --require-api-key appended unconditionally, and classify refuses to act on any AllowFunnel state (publish.rs:55,236-322,369-501); and every user-controlled value reaching a shell passes through shell_quote, with transport.rs:187-227 additionally rejecting hyphen-leading destinations. The author's status comment describes a push to 357130b, which is not reachable in this checkout, so I could not isolate what changed after it and reviewed the whole diff afresh instead — every factual claim in that comment that I could test against this head held up (all four handler shapes plus the converse are pinned; the held-port publish test is not vacuous; the two-record seeding makes the service_id conjunct load-bearing; the continuation merge is caught by its test; the provision.rs comment about install.sh:431 forcing verification regardless of channel is accurate). CI at this head: 19 success, 1 failure, 1 skipped — there is a failing check, and with no per-lane detail available I cannot attribute it to a named job. The checkout was left unmodified (git status clean; all mutation work was done in a separate scratch copy, also left clean). Blocking: 1 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

apps/rocm/src/remote/session.rs:371-397 — a_record_is_replaced_atomically_so_a_reader_never_sees_half_of_one cannot fail for the defect it names.

The test's name and its own leading comment claim it guards atomicity ("a torn record is skipped by load_all, which hides a session whose model is running and whose endpoint is published — the one thing that must never happen quietly"). It does not. Replacing the staging-file-plus-fs::rename sequence in RemoteSessionRecord::write (session.rs:108-133) with a plain fs::write(&path, &bytes) — deleting the atomicity mechanism outright — leaves the whole module green: cargo test -p rocm --bin rocm remote::session:: returned 9 passed; 0 failed with the mutation applied.

Why it passes regardless: the test writes twice, sequentially, in one thread. Nothing races and nothing is interrupted, so no torn file is ever produced. Assertion 1 (loaded.len() == 1) and assertion 2 (loaded[0].model == "a-different-model") hold for any correct overwrite, atomic or not. Assertion 3 scans for filenames containing "tmp" — under the mutation no staging file is ever created, so it passes vacuously. What the test actually proves is overwrite-by-deterministic-id, which is real and worth keeping, but it is not atomicity; CI currently certifies a guarantee nothing exercises.

Concrete fix — either of:

• Keep this test but rename it to what it checks (e.g. rewriting_a_session_replaces_the_record_rather_than_accumulating_one), drop the atomicity language from its comment, and add a separate test that can actually fail: plant a stray <id>.<pid>.<ts>.tmp file containing invalid JSON directly in the sessions directory and assert load_all still returns the real record and does not warn on it; plus a concurrency test spawning two threads calling write for the same id in a loop and asserting every interleaved load_all observation parses as complete, valid JSON.
• Or drive a fault through the mechanism: make the rename fail (e.g. by pre-creating a directory at the record path) and assert the error surfaces and the staging file is cleaned up — that branch at session.rs:129-133 is likewise unexercised today.

The whole point of the surrounding design comment is that a half-written record silently hides a live, published endpoint. That is exactly the failure this test is the only guard against, and it is not guarding it.

Non-blocking

• apps/rocm/src/remote/mod.rs:396-442 — the two unwind branches in serve_with_transport (publish failure, record-write failure) are never driven end-to-end; unwind_partial_serve is well tested directly with hand-fed arguments, but deleting the if let Err(...) = publish::publish(...) block would compile and leave the suite green, silently proceeding to print success over a model holding a GPU with no endpoint.
• apps/rocm/src/remote/doctor.rs:200-204 — is_stateful_sequence has no test at all; mutating it to always return false survives the module, and the consequence is a multi-step subshell fix rewritten into independent ssh calls that look runnable and are not.
• tests/e2e-cucumber/tests/e2e/remote_steps.rs:663-668 — remote-12's "endpoint is restored" Then matches CLI stdout only, while the sibling then_machine_publishing already has a container-side oracle; the behaviour itself is genuinely pinned by attach_republishes_a_healthy_session_whose_endpoint_went_missing (mod.rs:1932), so this is an inconsistency rather than a hole.
• .github/workflows/ci.yml:669 — the new remote-ssh job puts the heavy check in its job-level if, which this file's own comments warn against three times (verbatim, around line 1061: "Gating heavy at the job level would SKIP the job on a non-heavy PR, and a required check that is never produced stalls the merge queue"); harmless today, but a prerequisite to fix before the lane can be made required.
• apps/rocm/src/remote/provision.rs:39 — INSTALLER_URL is hardcoded to the public upstream main, ignoring the repo/mirror overrides install.sh itself supports, and the remote $HOME/.rocm/provision staging directory is never cleaned up after a successful install (the local side is, via Drop).

🔴 Automated review · pr-review-watcher · 9c8edd9

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

Adds rocm remote — serving a model on a tailnet GPU machine over an SSH control channel, with a Gherkin feature, a containerised SSH harness and a new CI lane. Outcome: Needs work — the production code held up well under adversarial review, but three tests do not constrain the branch they name. Verified: in a scratch copy, four branch-level mutation rounds of cargo test -p rocm --bin rocm filtered to the remote:: modules; the previously blocking atomicity gap is now genuinely caught (deleting the staging-file-plus-rename makes replacing_a_record_swaps_the_file_rather_than_rewriting_it_in_place fail — 10 passed, 1 failed, versus 9 passed 0 failed before), while deleting the peer sort, the clear_key id guard and the validate_destination leading-dash guard each left every one of the 156 remote:: tests green; re-confirmed on the live code that the API key reaches the remote only on stdin and never in argv or a command string, that no production path weakens SSH host-key checking, that checksum and signature are verified before the artifact is extracted or installed, that the endpoint is refused rather than published when Funnel is allowed and an API key is minted unconditionally, and that user-controlled values are shell-quoted. The full suite, the e2e suite and clippy were not run here. CI at this head: 18 success, 1 failure, 1 pending, 1 skipped — there is a failing check, and with no per-lane detail available I cannot attribute it to a named job, so nothing below rests on it. Blocking: 3 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

apps/rocm/src/remote/tailnet.rs:452 — parsing_orders_peers_and_strips_the_magicdns_trailing_dot does not test the ordering it names.
parse_status deserialises peers into a BTreeMap<String, RawPeer> keyed by node public key (tailnet.rs:136) and then explicitly sorts by host name (tailnet.rs:225-230). The fixture's keys are nodekey:aaa → gpu-box-1, nodekey:bbb → gpu-box-2, nodekey:ccc → phone, so BTreeMap iteration already yields exactly the expected ["gpu-box-1", "gpu-box-2", "phone"]. I deleted the sort_by call outright in a scratch copy: all 15 remote::tailnet tests still passed, including this one. The sort is correct production code guarding against unordered output, and nothing pins it. Fix: invert the fixture so key order and host order disagree — map nodekey:aaa to phone and nodekey:ccc to gpu-box-1, leaving the expected vector unchanged. Add a one-line comment saying the keys are deliberately in the opposite order to the hosts, so the next editor does not "tidy" the fixture back into agreement and silently re-blind the test.

apps/rocm/src/remote/session.rs:535 — an_unusable_id_cannot_place_a_credential_outside_the_sessions_directory asserts nothing about the behaviour its comment claims to catch.
clear_key guards a path-traversal by refusing ids that fail validate_id (session.rs:210-213). The test calls clear_key(&paths, "../../escaped") under the comment "And clearing one is a no-op rather than a delete somewhere else" and then makes no assertion at all — clear_key returns () and swallows the I/O result, so the line passes vacuously. I deleted the validate_id guard in a scratch copy, making clear_key unconditionally fs::remove_file a traversed path: every remote:: test still passed. This guards deletion of a file outside the sessions directory via an attacker-shaped session id, so it must be pinned. Fix: plant a file at the resolved escape target, call clear_key with the traversal id, and assert the planted file still exists.

apps/rocm/src/remote/transport.rs:626 — a_name_ssh_would_read_as_an_option_is_refused leaves half of the guard it names unpinned.
validate_destination refuses destination.starts_with('-') || host.starts_with('-') (transport.rs:192), where host is the part after the last @. Every hostile case in the test (-oProxyCommand=touch /tmp/pwned, --fake, user@-oX, "") is caught by the host disjunct alone, because each either contains no @ or puts the hyphen after it. I deleted the destination.starts_with('-') disjunct in a scratch copy: all 156 remote:: tests still passed. The shape that needs the deleted disjunct — -oProxyCommand=x@host, whose host half is clean but whose whole token ssh reads positionally as an option — is exactly the local-command-execution case the doc comment above the function describes. Fix: add "-oProxyCommand=touch /tmp/pwned@host" to the hostile list.

Non-blocking

• apps/rocm/src/remote/provision.rs:166-180 — no test drives push_matched_artifact, so deleting the .sha256/.sig sidecar pushes leaves all 15 remote::provision tests green (confirmed by mutation); the remote installer fails closed on a missing checksum, which is why this is not blocking, but the comment's "splitting the trust chain across two machines must not shorten it" is currently unenforced.
• apps/rocm/src/remote/publish.rs:681 — ports_are_matched_as_string_keys_not_numbers cannot distinguish a string-vs-numeric key regression from any other lookup mismatch.
• apps/rocm/src/remote/transport.rs:51 — exit_label's signal branch (code: None) is never exercised, so swapping the two arms would go unnoticed.
• tests/remote-ssh/Dockerfile:1 — alpine:3.20 is a floating tag; the repo already requires SHA-pinning for Actions, and digest-pinning the test base image would match that intent.
• Prior non-blocking items at the preceding commit: the serve_with_transport unwind branches are now driven end to end by dedicated tests and that item is resolved; is_stateful_sequence is still untested, remote-12's endpoint-restored step still matches stdout only (defensible — publish re-queries and bails unless the remote confirms), INSTALLER_URL is still hardcoded to a public HTTPS path with the remote staging directory still uncleaned, and the new CI lane's gating now reads consistently with the harness scripts.

Pushed. Both findings from your review are fixed.

assess() ignoring out_of_scope — you're right, and the reason it's easy to miss is that diagnose() returns an empty matched in two opposite situations: a machine that was checked and is clean, and a platform with no catalog entries where nothing runs at all. rocm-core says so in prose — "nothing was checked -- this is not a clean bill of health" — and assess read only matched. Now a NotEvaluated refusal, checked before findings and before sudo, carrying the catalog's own sentence. Tested at the call site too: ensure_ready_with(.., install_rocm = true) must never issue install driver.

The exists() / write race — also right, and worse than stated. exists() runs before ensure_ready_with, so the window isn't a few instructions, it's the whole readiness probe, which can take minutes when it provisions a CLI. The credential write is now the claim itself — create_new, so O_EXCL — with a distinct error so the loser can be told from a full disk and correctly skips clear_key on a key it never owned. The test races sixteen concurrent serves and asserts exactly one returns Ok; a filesystem-end-state assertion cannot see this defect, because both runs write to the same name.

Four invariants nothing currently observes, each demonstrated by deleting the mechanism and watching the suite stay green rather than by argument:

• Adding a verb to the daemon's read-only allowlist and not the CLI's leaves all 31 test binaries passing. Each side's test catches a known verb changing category; neither catches a new verb added on one side — which is the failure that already happened once here.
• Renaming the --require-api-key clap flag leaves 823 tests green while rocm remote serve would send a flag the CLI rejects. That string is the mechanism behind this PR's "never unauthenticated" claim, and nothing joins the two ends.
• The test named for services list --json agreeing with the table pins each side separately and never compares them.
• Only the Rust side of the signing-key precedence is pinned; resolve_public_keys could be reordered and only a comment would notice.

All four are cross-boundary, and none is in the diff's behaviour — they are gaps in what is defended. Whether they belong in this PR or as follow-ups is your call: at ~10.8k lines across 31 files it is past what one review pass covers, and you have already said as much.

On your five minors — none addressed yet. One I would flag as more than minor: transport.rs's broken-pipe suppression depends on the read running first and the exit status reflecting it, and with ; the compound's status is whatever rocm serve returns. That invariant is currently held by a comment rather than by the shell, which is exactly the coupling the comment warns about.

🔴 Automated review · pr-review-watcher · b2c5dd9

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

Adds a rocm remote command family that provisions, serves and publishes a model on a tailnet GPU machine over SSH, plus a containerised SSH harness and Gherkin coverage. Outcome: Needs work — one test does not constrain the mechanism it names, which is a hard blocker; everything else found is non-blocking. Verified: the remote:: unit filter is green at this head (162 passed) and I ran nine branch-level mutations — O_EXCL credential claim, race-loser clear_key skip, store_key id guard, the out_of_scope refusal (both its presence and its ordering ahead of findings/sudo), and the validate_destination leading-dash guard are each now caught by a named test; deleting the peer sort and deleting the clear_key id guard each left all 162 green. The full test suite, clippy and the e2e suite were not run here. Leak scan over the diff is clean, and no prompt-injection content was found in any reviewed file. Check-run conclusions this review worked from: 19 success, 1 failure, 1 skipped, 0 pending — I did not attribute the failure to any job. Blocking: 1 · Non-blocking: 5.

Previous round

assess() ignoring out_of_scope — genuinely resolved, and well-tested. install.rs:67-71 returns Refusal::NotEvaluated carrying the catalog's own sentence, ahead of the findings check and the sudo check. Deleting that early return fails four tests, including the call-site test that asserts ensure_ready_with(.., install_rocm = true) never issues install driver. Separately, moving the check to after findings and after sudo — keeping it present — still fails an_unscored_platform_outranks_a_sudo_problem, so the ordering claim is tested and not merely asserted.

The exists()/write race — genuinely resolved. session.rs:251 claims the name with create_new (O_EXCL); removing the exclusivity fails two tests, including the sixteen-thread race. Making the race loser call clear_key also fails that race test. The exists() call at mod.rs:298 remains, but only selects wording inside an already-failing branch — it is no longer load-bearing.

One correction to the author's reasoning: the loser skipping clear_key is not produced by the distinct error type. Folding AlreadyExists back into the generic io error leaves all 162 tests green, because neither arm of that early-return block calls clear_key — the skip is structural. The distinct type's only observable effect is the message the loser sees, and nothing tests that.

Of the three previously-reported non-constraining tests, one is fixed and two are not. The validate_destination leading-dash guard is now constrained (deleting it fails a_name_ssh_would_read_as_an_option_is_refused). The peer sort is still unconstrained — see Blocking. The clear_key id guard is still unconstrained — see Non-blocking.

🚫 Blocking (must fix before merge)

apps/rocm/src/remote/tailnet.rs:453 — parsing_orders_peers_and_strips_the_magicdns_trailing_dot names the peer ordering and asserts ["gpu-box-1", "gpu-box-2", "phone"], but cannot fail when the ordering is removed. RawStatus.peers is a BTreeMap keyed by nodekey, and the fixture's keys (nodekey:aaa → gpu-box-1, nodekey:bbb → gpu-box-2, nodekey:ccc → phone) are already in host-alphabetical order, so into_values() yields exactly the asserted sequence before peers.sort_by(...) at tailnet.rs:225 ever runs. Deleting lines 225-230 leaves all 162 remote:: tests green — measured, not argued. No Gherkin scenario covers peer ordering either, so nothing else catches it. This is the same defect class the previous round reported for this test, and it is unchanged at this head. Fix: permute the fixture so nodekey order and host order disagree — give the alphabetically-last host the alphabetically-first nodekey (e.g. nodekey:aaa → phone, nodekey:ccc → gpu-box-1) — so the assertion depends on the explicit sort. Two further assertions in the same test (peers[0].dns_name, the trailing-dot strip) are indexed by position and would become genuinely order-dependent at the same time.

Non-blocking

• apps/rocm/src/remote/session.rs:639 — clear_key(&paths, "../../escaped") is followed by no assertion, so the comment's claim that "clearing one is a no-op rather than a delete somewhere else" is unverified; deleting the guard at session.rs:277 leaves all 162 tests green. Raised in the previous round, unaddressed. The store side of the same test is constrained. Cheap fix: write a sentinel file above the sessions directory and assert it survives the call. The guard is defence-in-depth rather than a live hole — read_one validates ids on read — which is worth saying in the comment so the next reader does not have to rediscover it.
• apps/rocm/src/remote/transport.rs:451-456 — the comment justifies the broken-pipe demotion with "a broken pipe means the read never finished, which means the command cannot have exited 0". That does not hold: remote_serve_command joins with ;, and the piped key carries no trailing newline, so IFS= read -r returns nonzero at EOF whether it read the whole key or a truncated one, and ; lets the chain continue regardless. The code is safe anyway — the && !output.status.success() conjunct catches a truncated key paired with a successful serve — but the stated invariant is false, and the comment's own "reorder that command and this demotion needs rechecking" invites a maintainer to trust it. State the real invariant instead.
• apps/rocm/src/remote/publish.rs:479 — after the off, PublishState::Foreign is folded into Ok(()) alongside Absent, uncommented, unlike every other arm in both matches in this function. The pre-check correctly refuses Foreign, so the realistic case is handled and the residual window is inherent (tailscale serve … off takes a port, with no compare-and-swap), but a foreign forward appearing on the port after our teardown is not the same fact as a clean withdrawal and should not read as one. Separate the arms, or at minimum warn.
• .github/workflows/ci.yml:669 — the new remote-ssh job carries only needs: changes, while build-and-test, test and windows-build-and-test all carry needs: [changes, clippy, prek] plus if: github.event_name != 'workflow_dispatch', with a comment explaining both. This job builds and runs a 30-minute container suite, so it is the same weight class; nothing in its own comments explains the deviation. Either adopt the sibling pattern or record why it differs.
• apps/rocm/src/remote/provision.rs:160 — the remote staging directory is created with a plain mkdir -p and never tightened, while the local side uses an atomic DirBuilder::mode(0o700) whose surrounding comment argues carefully for exactly that. The contents are public release material, not secrets, so the exposure is narrow, but mkdir -p -m 700 costs nothing and matches the stated intent.

Check-run conclusions were re-read immediately before posting and are unchanged from the counts stated above.